Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conda Fetch Plugin Hook #44

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

travishathaway
Copy link
Contributor

This CEP proposed a new plugin hook that will allow plugin authors to completely customize the way that network requests are made. Please check what is currently on the CEP and use this pull request as a forum to discuss changes or oppositions to it.

new-cep.md Outdated Show resolved Hide resolved
new-cep.md Outdated Show resolved Hide resolved
travishathaway and others added 5 commits December 20, 2022 10:20
Co-authored-by: Bianca Henderson <beeankha@gmail.com>
This is more or less ready for review. I still need to add some FAQs and
there are probably grammar errors, but the spirit of what I want to
convey is there.
…away/ceps into cep-conda-session-pluing-hook
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the hook has me worried here, it's a little Russian doll style naming confusion IMO and visible in the example: CondaSessionClass has a session_class attribute of subclasses of CondaSession.

I wonder if we should use "session class" for the plugin name, as it's an implementation detail of the requests library and not a feature of conda itself. It feels too overused for a plugin hook and I think precludes what this could be used for.

I think a few things could work to balance this out:

  • CondaSession should be renamed to CondaRequestsSession since that's what it is and would prevent 3rd party devs to accidentally try to mimic that class instead of the requests.Session class.
  • Use "requester" or "transmitter" as the domain name of this new plugin hook.
    So that conda install --transmitter=pycurl scipy works

The example would look like this:

class CustomCondaRequestsSession(CondaRequestsSession):
     """
     Our custom CondaSession class which defines an additional class
     property
     """
     def __init__(self):
         super().__init__()
         self.custom_property = 'custom_property'


 @hookimpl
 def conda_transmitters(): 
     """
     Register our custom CondaRequestsSession class
     """
     yield CondaTransmitter(
         name=PLUGIN_NAME, 
         session=CustomCondaRequestsSession
     )

The UX would be:

$ CONDA_TRANSMITTER=pycurl conda install scipy
$ conda install --transmitter=pycurl scipy

new-cep.md Outdated
Those are just a few examples of authentication schemes this plugin
would enable conda to support. Because there are many more, it would be
unfeasible and difficult to maintain each and every one if we decided to
add these all as custom authentication schemes to conda itself. This is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also have to implement entirely new authentication schemes in the future, that currently don't exist.

new-cep.md Outdated Show resolved Hide resolved
new-cep.md Outdated Show resolved Hide resolved
new-cep.md Outdated

## FAQs

_Any FAQs will go here. Plus add any in the comments of the pull request!_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "Why did you choose to use requests' Session class as the basis for this plugin hook?"
  • "What are the expected behavior inherent 3rd party conda session classes?" (e.g. thead-safety etc)

new-cep.md Outdated Show resolved Hide resolved
@jezdez
Copy link
Member

jezdez commented Dec 20, 2022

BTW as some inspiration for people creating requests-style session classes: https://github.com/dcoles/pycurl-requests

Co-authored-by: Jannis Leidel <jannis@leidel.info>
@travishathaway
Copy link
Contributor Author

@jezdez,

I really like your "transmitter" idea. You pointed something out that I felt more and more awkward about as I was writing the plugin prototype, which is the naming of the CondaSession class. I'd also like to rename that class in any implementation we go with too.

Also, thank you for adding those high level UX examples. I will definitely make room for those in the CEP too (under "Specification").

@travishathaway travishathaway deleted the cep-conda-session-pluing-hook branch December 21, 2022 12:38
@travishathaway travishathaway restored the cep-conda-session-pluing-hook branch December 21, 2022 12:38
@travishathaway
Copy link
Contributor Author

@jezdez,

I switched this back to fetch for now 😅 . It's shorter and easier to type than transmitter. It also aligns with the original "spirit" of this work (whatever that means 😂).

@travishathaway travishathaway changed the title Add plugin hook for overriding network requests in conda Conda Fetch Plugin Hook Dec 22, 2022
@travishathaway travishathaway marked this pull request as ready for review January 3, 2023 13:59
new-cep.md Outdated Show resolved Hide resolved
new-cep.md Outdated
Comment on lines 103 to 107
```yaml
channels:
- https://my-custom-conda-packages.com:
fetch: custom_fetch
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this be achieved via the CLI or env vars?

Copy link
Contributor Author

@travishathaway travishathaway Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaimergp,

I am not sure there is a clean way to do this that would be the most flexible.

Here's an example of a method one could use on the command line to effectively achieve the the configuration defined above:

conda install python \
  --channels "https://my-custom-conda-packages.com" \
  --fetch custom_fetch \
  --override-channels

and with environment variables:

export CONDA_CHANNELS="https://my-custom-conda-packages.com"
export CONDA_OVERRIDE_CHANNELS=1
export CONDA_FETCH=custom_fetch
conda install new_package

The only problem is that this approach breaks as soon as you want to add another channel in there (e.g. "conda-forge"). For the most flexible option, I think passing in JSON as the value for channel makes the most sense:

conda install python \
  --channels '{"https://my-custom-conda-packages.com": {"fetch": "custom_fetch"}}'

and as an environment variable:

export CONDA_CHANNELS='{"https://my-custom-conda-packages.com": {"fetch": "custom_fetch"}}'

That is definitely not the prettiest approach, but it Works™️. It lines up with changes that we are currently proposing for channel configuration (conda/conda#12033).

Do you have any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, after some thinking I think we would need JSON support for maps. I don't know if that can become a problem by being too flexible, though. Nothing Pydantic validation and schemas cannot help with :D

travishathaway and others added 2 commits January 9, 2023 13:11
Co-authored-by: Jaime Rodríguez-Guerra <jaimergp@users.noreply.github.com>
@travishathaway
Copy link
Contributor Author

I wonder if we should be including support for adding additional adapters here too:

Perhaps it makes sense to be able to expose just these as hooks too.

@milljm
Copy link

milljm commented Feb 16, 2023

I would be interested in this. We ended up creating a mock object for our session purposes. It would be nice to move to something more supported by the community.

    with mock.patch('conda.gateways.connection.session.CondaSession',
                    return_value=CondaSessionRSA()):

@travishathaway
Copy link
Contributor Author

We recently had a new feature request in conda repository that could also be supported by this new plugin hook:

This would also be a good case for allowing plugin authors for adding additional adapters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants